[Mobile Payments] Disconnect built-in reader when Manage card readers is tapped #8666

joshheald · 2023-01-17T12:25:29Z

Closes: #8664

Description

In sTAP Away, we add support for the built-in card reader, allowing merchants to take card payments using Tap to Pay on iPhone.

We can be connected to either the built-in reader, or a Bluetooth reader, but not both simultaneously.

There’s nothing to manage or view about the built in reader, and from a user’s perspective there isn’t a reader at all, it’s just part of their phone.

Since we can’t connect to the Bluetooth reader until the built in reader is disconnected, we just do that automatically for the user here. This saves us updating the display of the connected reader to be suitable for the built in reader too.

We can revisit this in future, but for M1 it makes sense.

Testing instructions

Using a US-based WCPay or Stripe store:

Navigate to Menu > Settings > Experimental features
Turn on Tap to Pay on iPhone
Navigate to Menu > Payments > Collect payment
Go through the payment flow, and select Card on the payment method screen
When asked for a reader type, tap Tap to Pay on iPhone and connect to the reader
Take a payment.
When it's finished, repeat the above three steps to take another payment: observe that you don't need to connect to the reader and are not asked for a reader type.
Go to Menu > Payments > Manage Card Reader
Observe that there is no reader shown as connected
Repeat the above steps to take a payment: observe that you are asked to choose a reader type again, and can connect to the reader as the first time.

In general –

Observe that TTPoI connections persist as long as the app is in the foreground, unless you tap Manage Card Reader
Observe that Bluetooth connections persist until you tap Disconnect in the Manage Card Reader screen.

Screenshots

disconnect-on-manage-reader.mp4

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

There’s nothing to manage or view about the built in reader, and from a user’s perspective there isn’t a reader at all, it’s just part of their phone. Since we can’t connect to the Bluetooth reader until the built in reader is disconnected, we just do that automatically for the user here. This saves us updating the display of the connected reader to be suitable for the built in reader too. We can revisit this in future, but for M1 it makes sense.

wpmobilebot · 2023-01-17T12:42:52Z

You can test the changes from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr8666-e3f9fcc on your iPhone

If you need access to App Center, please ask a maintainer to add you.

toupper · 2023-01-17T17:18:37Z

Thanks for the PR @joshheald! I have a question regarding the implementation, also link with my first comment.

If I follow the flow correctly, every time the list of connected readers changes we are notified by observing the observeConnectedReaders action completion block. Once it changes, we call to update the properties, where we disconnect the first reader if it's the Apple Built-in one.

Is it like that? In that case, what happens if the list changes because the Apple Built-in reader is just connected? From this perspective, we can end up having the opposite effect we expected: We wanted to connect the Apple Built-in reader, but it ends up being disconnected.

toupper · 2023-01-17T17:03:17Z

...sses/ViewRelated/Dashboard/Settings/CardReadersV2/CardReaderSettingsConnectedViewModel.swift

    }

    private func updateProperties() {
+        if let connectedReader = connectedReaders.first,


I am wondering, is this the best place to disconnect the reader? After all, the purpose of this function is to update the properties with the first connected reader data; disconnecting the reader seems like an unexpected side effect here.

Fair point: I could put it directly in the completion block for observeConnectedReaders in beginObservation. It won't make any difference to the functionality or way it works though: updateProperties is only called from that same block.

Moved in 0a92d0f

toupper · 2023-01-17T17:06:06Z

...sses/ViewRelated/Dashboard/Settings/CardReadersV2/CardReaderSettingsConnectedViewModel.swift

            newShouldShow = .isUnknown
        } else if connectedReaders.isEmpty {
            newShouldShow = .isFalse
+        } else if connectedReaders.first(where: { $0.readerType == .appleBuiltIn }) != nil {


Technically this means that if any of the connected readers is the Apple Built-in one we don't show the view right? What happens if we are connected to a Bluetooth reader apart from of that?

Correct, however we don't actually support being connected to more than one reader, and neither do Stripe.

So we can't be connected to more than one at a time, and this class doesn't otherwise handle that situation anyway (it relies on .first to determine the connected reader.)

The proper fix for this inconsistency would be to change connectedReaders to a single object, not an array, but that is way out of scope here 😊

The proper fix for this inconsistency would be to change connectedReaders to a single object, not an array, but that is way out of scope here 😊

That's what I thought, can we create an issue to change that?

toupper · 2023-01-17T17:06:50Z

WooCommerce/WooCommerceTests/Mocks/MockCardPresentPaymentsStoresManager.swift

    private var softwareUpdateSubject: CurrentValueSubject<CardReaderSoftwareUpdateState, Never> = .init(.none)
    private var paymentExtension: CardPresentPaymentGatewayExtension

+    var recievedActions: [CardPresentPaymentAction] = []


Typo here, recievedActions -> receivedActions

Fixed in 791b53b

joshheald · 2023-01-17T17:24:04Z

@toupper Thanks for the review

Is it like that? In that case, what happens if the list changes because the Apple Built-in reader is just connected? From this perspective, we can end up having the opposite effect we expected: We wanted to connect the Apple Built-in reader, but it ends up being disconnected.

This is not a problem: the code in question is specific to the Manage card reader screen, and we do not allow merchants to connect to the built-in reader from that screen. The Connect card reader button on there will only trigger a Bluetooth scan, not a built-in reader connection.

This is because the merchant is unlikely to think of the built-in reader as a "Card reader" they need to manage, it's just a part of their phone.

toupper · 2023-01-18T09:13:33Z

This is not a problem: the code in question is specific to the Manage card reader screen, and we do not allow merchants to connect to the built-in reader from that screen. The Connect card reader button on there will only trigger a Bluetooth scan, not a built-in reader connection.

This is because the merchant is unlikely to think of the built-in reader as a "Card reader" they need to manage, it's just a part of their phone.

Thanks for your answer @joshheald ! That makes sense now. I have a few suggestions to make this more clear:

Rename CardReaderSettingsConnectedViewModel to BluetoothCardReaderSettingsConnectedViewModel. That way we know that that screen is only for Bluetooth readers.
Move the code

if readers.first(where: { $0.readerType == .appleBuiltIn }) != nil {
                self.disconnect()
            }

to its own function, with naming disconnectAppleBuiltInReader, so we see what's being done here.

Add a comment in that function to describe why we need to do that

toupper · 2023-01-18T09:31:41Z

@joshheald The code looks good and it tests well 🎉 I added a few code suggestions, and I have one small finding when testing:

Repeat the above steps to take a payment: observe that you are asked to choose a reader type again, and can connect to the reader as the first time.

When testing this step, not only are we asked what reader we want to connect, but we also show a loading screen with "Connecting to your account" message. If the reader is connected (step 7) we don't need to connect to our account. Why is being disconnected from a reader causes having to connect to the account?

joshheald · 2023-01-18T11:04:14Z

Thanks for the re-review @toupper

When testing this step, not only are we asked what reader we want to connect, but we also show a loading screen with "Connecting to your account" message. If the reader is connected (step 7) we don't need to connect to our account. Why is being disconnected from a reader causes having to connect to the account?

This happens in the onboarding check, it's pre-existing. We always check the onboarding status before any reader connection, so that we can be sure the user's account is in the correct state to connect/take a payment, but we don't do that if they're already connected to a reader as we can presume that they've had the check done recently.

The CardReaderSettingsConnectedViewModel is now specific to Bluetooth readers, as we do not expect users to view the built-in card reader as something they need to connect to and manage. This commit neatens that up and explains the logic.

joshheald · 2023-01-18T11:15:09Z

@toupper Bluetooth-specific updates in e3f9fcc, thanks for the suggestions

joshheald added type: enhancement A request for an enhancement. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. status: feature-flagged Behind a feature flag. Milestone is not strongly held. labels Jan 17, 2023

joshheald added this to the 12.0 milestone Jan 17, 2023

joshheald marked this pull request as ready for review January 17, 2023 12:46

joshheald requested review from jostnes, lmischner and toupper January 17, 2023 12:46

toupper self-assigned this Jan 17, 2023

toupper reviewed Jan 17, 2023

View reviewed changes

joshheald added 2 commits January 17, 2023 17:35

8664 Fix typo in mock

791b53b

Move built-in reader disconnect so it’s not a side effect

0a92d0f

joshheald requested a review from toupper January 17, 2023 18:03

toupper approved these changes Jan 18, 2023

View reviewed changes

Base automatically changed from issue/8290-ttpoi-setup-cancellation to trunk January 18, 2023 10:34

joshheald mentioned this pull request Jan 18, 2023

[Mobile Payments] Change CardReaderService to make it clear we only support one connected reader #8683

Open

joshheald enabled auto-merge January 18, 2023 11:15

joshheald merged commit f58c9ac into trunk Jan 18, 2023

joshheald deleted the issue/8664-disconnect-from-built-in-reader-when-manage-card-readers-tapped branch January 18, 2023 11:40

[Mobile Payments] Disconnect built-in reader when Manage card readers is tapped #8666

[Mobile Payments] Disconnect built-in reader when Manage card readers is tapped #8666

Uh oh!

Conversation

joshheald commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Using a US-based WCPay or Stripe store:

Screenshots

Uh oh!

wpmobilebot commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toupper commented Jan 17, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshheald commented Jan 17, 2023

Uh oh!

toupper commented Jan 18, 2023

Uh oh!

toupper commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshheald commented Jan 18, 2023

Uh oh!

joshheald commented Jan 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joshheald commented Jan 17, 2023 •

edited

Loading

wpmobilebot commented Jan 17, 2023 •

edited

Loading

toupper commented Jan 18, 2023 •

edited

Loading